Skip to content

[dotnet] Make internal console writer more flexible via taking TextWriter only #15346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Feb 27, 2025
Merged

[dotnet] Make internal console writer more flexible via taking TextWriter only #15346

merged 5 commits into from
Feb 27, 2025

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 27, 2025 •
edited
Loading

User description

Console is an output. We can be more extendable/abstract/generic.

Motivation and Context

Taking minimal required.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement

Description

  • Replaced ConsoleLogHandler with TextWriterHandler for flexibility.

  • Updated LogContextManager to use TextWriterHandler.

  • Modified logging tests to reflect the new handler.

  • Improved abstraction by utilizing TextWriter for log output.

Changes walkthrough 📝

Relevant files
Enhancement
LogContextManager.cs
Refactored LogContextManager to use TextWriterHandler       

dotnet/src/webdriver/Internal/Logging/LogContextManager.cs

  • Replaced ConsoleLogHandler with TextWriterHandler.
  • Updated GlobalContext initialization to use the new handler.
  • Simplified AsyncLocal initialization syntax.
    		• +4/-3     
    TextWriterHandler.cs
    Introduced TextWriterHandler for flexible log output         

    dotnet/src/webdriver/Internal/Logging/TextWriterHandler.cs

  • Renamed ConsoleLogHandler to TextWriterHandler.
  • Replaced Console.Error with a TextWriter parameter.
  • Updated log writing logic to use TextWriter.
    		• +4/-4     
    Tests
    LogTest.cs
    Updated logging tests for TextWriterHandler                           

    dotnet/test/common/Internal/Logging/LogTest.cs

  • Updated tests to use TextWriterHandler instead of ConsoleLogHandler.
  • Adjusted global log reset logic to reflect handler changes.
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 27, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Resource Management

    The TextWriter instance passed to TextWriterHandler is not disposed or managed by the handler. Consider implementing IDisposable or documenting that the caller is responsible for managing the TextWriter lifecycle.

     
    public class TextWriterHandler(TextWriter writer) : ILogHandler
{

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 27, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check for writer

    Add null check for the TextWriter parameter to prevent NullReferenceException
    when writing logs.

    dotnet/src/webdriver/Internal/Logging/TextWriterHandler.cs [29-32]

     public class TextWriterHandler(TextWriter writer) : ILogHandler
 {
+    private readonly TextWriter _writer = writer ?? throw new ArgumentNullException(nameof(writer));
     // performance trick to avoid expensive Enum.ToString() with fixed length
     private static readonly string[] _levels = { "TRACE", "DEBUG", " INFO", " WARN", "ERROR" };
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding a null check for the TextWriter parameter is crucial for preventing potential NullReferenceException during runtime, which could cause application crashes. This is a important defensive programming practice for a core logging component.

    		Medium
    • Update

    RenderMichael
    RenderMichael requested changes Feb 27, 2025
    View reviewed changes
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Having the TextWriterHandler is a good idea, however the existing ConsoleLogHandler is used by users. We would need to keep a type that just points to the text writer handler:

    public class ConsoleLogHandler : TestWriterHandler(Console.Error);

    Additionally, TestWriterHandler(Console.Error) is not the same as ConsoleLogHandler if a user uses the Console.SetError method. The current code would point to whatever the new Console.Error pointed to, but this PR would still point to the old sink.

    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Feb 27, 2025 •
    edited
    Loading

    Additionally, TestWriterHandler(Console.Error) is not the same as ConsoleLogHandler if a user uses the Console.SetError method. The current code would point to whatever the new Console.Error pointed to, but this PR would still point to the old sink.

    True, and it is still good. As a low-level library we want to give user a power. Since this is "internal/advanced" thing, I am OK with this behavior.

    however the existing ConsoleLogHandler is used by users

    OK, let's keep it and proceed via standard deprecation policy. Thank you, it is really better than just break.

    RenderMichael
    RenderMichael reviewed Feb 27, 2025
    View reviewed changes
    @nvborisenko nvborisenko merged commit 5ca1cc0 into SeleniumHQ:trunk Feb 27, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-log-console-rework branch February 27, 2025 22:13
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @nvborisenko @sandeepsuryaprasad
    [dotnet] Make internal console writer more flexible via taking TextWr…
    2159cd5 
    …iter only (SeleniumHQ#15346)
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @RenderMichael RenderMichael RenderMichael approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @RenderMichael